chore: import secp256k1 for amino parsing #1178

albttx · 2023-09-28T09:55:21Z

Amino failed to parse crypto.PubKey

Description

This simple client isn't working to get account because amino don't know the struct BaseAccount.Pubkey crypto.PubKey

This simple main can be fixed by importing, i believe it will make dev experience easier by putting it in std rather than clients

_ "github.com/gnolang/gno/tm2/pkg/crypto/secp256k1"

package main

import (
	"fmt"

	"github.com/gnolang/gno/tm2/pkg/amino"
	rpcClient "github.com/gnolang/gno/tm2/pkg/bft/rpc/client"
	"github.com/gnolang/gno/tm2/pkg/std"
)


// Client is the TM2 HTTP client
type Client struct {
	client rpcClient.Client
}

// NewClient creates a new TM2 HTTP client
func NewClient(remote string) *Client {
	return &Client{
		client: rpcClient.NewHTTP(remote, ""),
	}
}

func (c *Client) GetAccount(address string) (std.Account, error) {
	path := fmt.Sprintf("auth/accounts/%s", address)

	queryResponse, err := c.client.ABCIQuery(path, []byte{})
	if err != nil {
		return nil, fmt.Errorf("unable to execute ABCI query, %w", err)
	}

	var queryData struct{ BaseAccount std.BaseAccount }

	if err := amino.UnmarshalJSON(queryResponse.Response.Data, &queryData); err != nil {
		return nil, err
	}

	return &queryData.BaseAccount, nil
}

func main() {
	c := NewClient("http://rpc.gnochess.com:80")
	acc, err := c.GetAccount("g1x90eh5ejc22548hjqznm2egyvn8ny36lqu460f")
	fmt.Println(acc, err)
}

runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0xc020780390 stack=[0xc020780000, 0xc040780000]
fatal error: stack overflow

runtime stack:
runtime.throw({0x9842fa?, 0xd4dea0?})
        /nix/store/akhjsmrrsakcnj8x3xgygvizhccbyn0v-go-1.19.3/share/go/src/runtime/panic.go:1047 +0x5d fp=0x7f681368cc18 sp=0x7f681368cbe8 pc=0x43907d
runtime.newstack()
        /nix/store/akhjsmrrsakcnj8x3xgygvizhccbyn0v-go-1.19.3/share/go/src/runtime/stack.go:1103 +0x5cc fp=0x7f681368cdd0 sp=0x7f681368cc18 pc=0x452d0c
runtime.morestack()
        /nix/store/akhjsmrrsakcnj8x3xgygvizhccbyn0v-go-1.19.3/share/go/src/runtime/asm_amd64.s:570 +0x8b fp=0x7f681368cdd8 sp=0x7f681368cdd0 pc=0x46a32b

goroutine 1 [running]:
fmt.(*fmt).padString(0xc00de1ff20?, {0x8961be, 0x7})
        /nix/store/akhjsmrrsakcnj8x3xgygvizhccbyn0v-go-1.19.3/share/go/src/fmt/format.go:108 +0x299 fp=0xc0207803a0 sp=0xc020780398 pc=0x4dac39
fmt.(*fmt).fmtS(0xc0207803f8?, {0x8961be?, 0x8961bd?})
        /nix/store/akhjsmrrsakcnj8x3xgygvizhccbyn0v-go-1.19.3/share/go/src/fmt/format.go:359 +0x3f fp=0xc0207803d8 sp=0xc0207803a0 pc=0x4db75f
fmt.(*pp).fmtString(0x8ca000?, {0x8961be?, 0x8ca000?}, 0x0?)
        /nix/store/akhjsmrrsakcnj8x3xgygvizhccbyn0v-go-1.19.3/share/go/src/fmt/print.go:474 +0x86 fp=0xc020780428 sp=0xc0207803d8 pc=0x4de566
fmt.(*pp).handleMethods(0xc00de1fee0, 0x100800?)
        /nix/store/akhjsmrrsakcnj8x3xgygvizhccbyn0v-go-1.19.3/share/go/src/fmt/print.go:65

[....] It's a stack overflow due to recursion

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

codecov · 2023-09-28T09:59:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5a0ff55) 56.30% compared to head (3bef735) 56.06%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1178      +/-   ##
==========================================
- Coverage   56.30%   56.06%   -0.24%     
==========================================
  Files         422      421       -1     
  Lines       65699    65457     -242     
==========================================
- Hits        36993    36700     -293     
- Misses      25821    25891      +70     
+ Partials     2885     2866      -19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

thehowl

LGTM; though we may want to wait on #1117 in order to add a regression test. (start node -> try go run on the small script you made)

moul

I approve the fix. I'm unsure about the ideal location for the anonymous import. Perhaps it would make more sense to place it in the rpc/client. It's fine to merge it from my end.

By the way, it would be great to have a small unit test. The test should involve creating a minimal client and attempting to unmarshal something with an account key.

piux2

This file is intended to be used as a library and does not contain a main() function. For compatibility with Amino, each package invokes Package = amino.RegisterPackage() at the package level. This registers Amino types with the global codec, referred to as gcdc

In other word the func main() suppose to use out side tm2/pkg.

thehowl · 2023-10-10T15:16:35Z

This file is intended to be used as a library and does not contain a main() function. For compatibility with Amino, each package invokes Package = amino.RegisterPackage() at the package level. This registers Amino types with the global codec, referred to as gcdc

In other word the func main() suppose to use out side tm2/pkg.

@piux2 Okay, so what solution are you proposing, I don't understand?

The issue is that there is a bad dev experience if somebody tries to amino.UnmarshalJSON from a very basic example which Albert has kindly provided. I understand why we need the blank import (the amino.RegisterPackage call you mentioned); but what this PR is about is adding "sane defaults". (Obviously, part of the issue is that the error message is incredibly unhelpful, but this was fixed by #1179).

Can you make a compelling case for why the users should do the blank import, rather than having this transitively imported when importing either std or rpc/client packages? (Or any other place which makes sense, imported by Albert's sample file?)

piux2 · 2023-10-23T01:56:17Z

This file is intended to be used as a library and does not contain a main() function. For compatibility with Amino, each package invokes Package = amino.RegisterPackage() at the package level. This registers Amino types with the global codec, referred to as gcdc
In other word the func main() suppose to use out side tm2/pkg.

@piux2 Okay, so what solution are you proposing, I don't understand?

The issue is that there is a bad dev experience if somebody tries to amino.UnmarshalJSON from a very basic example which Albert has kindly provided. I understand why we need the blank import (the amino.RegisterPackage call you mentioned); but what this PR is about is adding "sane defaults". (Obviously, part of the issue is that the error message is incredibly unhelpful, but this was fixed by #1179).

Can you make a compelling case for why the users should do the blank import, rather than having this transitively imported when importing either std or rpc/client packages? (Or any other place which makes sense, imported by Albert's sample file?)

@thehowl The short answer is

crypto.PubKey is an interface, not a struct.

We want to load all concrete type of the PubKey in account.go so that our code and can automatically pick the right concrete PubKey type when we unmarshal the std.BaseAccount

_ "github.com/gnolang/gno/tm2/pkg/crypto/secp256k1"
_ "github.com/gnolang/gno/tm2/pkg/crypto/ed25519"
_ "github.com/gnolang/gno/tm2/pkg/crypto/multisig"
_ "github.com/gnolang/gno/tm2/pkg/crypto/mock"

thehowl · 2023-10-23T17:57:05Z

We want to load all concrete type of the PubKey in account.go so that our code and can automatically pick the right concrete PubKey type when we unmarshal the std.BaseAccount

Alright, sorry, sometimes hard to understand your concrete proposal :) (pun very intended)

Agreed on your plan of action!

albttx · 2023-10-24T15:50:49Z

Thanks @piux2 i love the idea, just updated the list of imported packages

piux2 · 2023-10-25T19:43:02Z

Thanks @piux2 i love the idea, just updated the list of imported packages

@albttx Good work!

Let's add import these in std/account.go. It is close to crypto.PubKey used in the BaseAccount
The package.go file in each directly is purely used for genproto to generate Amino compatible proto message file.

thehowl · 2023-12-07T12:20:51Z

ping @albttx for integrating @piux2's requested changes :)

…t-get-account

albttx · 2023-12-07T12:26:45Z

@thehowl merged master, formatted and udpated :)

albttx · 2023-12-07T17:13:57Z

@piux2 Waiting for your approval for a merge :)

## Amino failed to parse crypto.PubKey ### Description This simple client isn't working to get account because amino don't know the struct `BaseAccount.Pubkey crypto.PubKey` This simple main can be fixed by importing, i believe it will make dev experience easier by putting it in `std` rather than clients ``` _ "github.com/gnolang/gno/tm2/pkg/crypto/secp256k1" ``` ```go package main import ( "fmt" "github.com/gnolang/gno/tm2/pkg/amino" rpcClient "github.com/gnolang/gno/tm2/pkg/bft/rpc/client" "github.com/gnolang/gno/tm2/pkg/std" ) // Client is the TM2 HTTP client type Client struct { client rpcClient.Client } // NewClient creates a new TM2 HTTP client func NewClient(remote string) *Client { return &Client{ client: rpcClient.NewHTTP(remote, ""), } } func (c *Client) GetAccount(address string) (std.Account, error) { path := fmt.Sprintf("auth/accounts/%s", address) queryResponse, err := c.client.ABCIQuery(path, []byte{}) if err != nil { return nil, fmt.Errorf("unable to execute ABCI query, %w", err) } var queryData struct{ BaseAccount std.BaseAccount } if err := amino.UnmarshalJSON(queryResponse.Response.Data, &queryData); err != nil { return nil, err } return &queryData.BaseAccount, nil } func main() { c := NewClient("http://rpc.gnochess.com:80") acc, err := c.GetAccount("g1x90eh5ejc22548hjqznm2egyvn8ny36lqu460f") fmt.Println(acc, err) } ``` ``` runtime: goroutine stack exceeds 1000000000-byte limit runtime: sp=0xc020780390 stack=[0xc020780000, 0xc040780000] fatal error: stack overflow runtime stack: runtime.throw({0x9842fa?, 0xd4dea0?}) /nix/store/akhjsmrrsakcnj8x3xgygvizhccbyn0v-go-1.19.3/share/go/src/runtime/panic.go:1047 +0x5d fp=0x7f681368cc18 sp=0x7f681368cbe8 pc=0x43907d runtime.newstack() /nix/store/akhjsmrrsakcnj8x3xgygvizhccbyn0v-go-1.19.3/share/go/src/runtime/stack.go:1103 +0x5cc fp=0x7f681368cdd0 sp=0x7f681368cc18 pc=0x452d0c runtime.morestack() /nix/store/akhjsmrrsakcnj8x3xgygvizhccbyn0v-go-1.19.3/share/go/src/runtime/asm_amd64.s:570 +0x8b fp=0x7f681368cdd8 sp=0x7f681368cdd0 pc=0x46a32b goroutine 1 [running]: fmt.(*fmt).padString(0xc00de1ff20?, {0x8961be, 0x7}) /nix/store/akhjsmrrsakcnj8x3xgygvizhccbyn0v-go-1.19.3/share/go/src/fmt/format.go:108 +0x299 fp=0xc0207803a0 sp=0xc020780398 pc=0x4dac39 fmt.(*fmt).fmtS(0xc0207803f8?, {0x8961be?, 0x8961bd?}) /nix/store/akhjsmrrsakcnj8x3xgygvizhccbyn0v-go-1.19.3/share/go/src/fmt/format.go:359 +0x3f fp=0xc0207803d8 sp=0xc0207803a0 pc=0x4db75f fmt.(*pp).fmtString(0x8ca000?, {0x8961be?, 0x8ca000?}, 0x0?) /nix/store/akhjsmrrsakcnj8x3xgygvizhccbyn0v-go-1.19.3/share/go/src/fmt/print.go:474 +0x86 fp=0xc020780428 sp=0xc0207803d8 pc=0x4de566 fmt.(*pp).handleMethods(0xc00de1fee0, 0x100800?) /nix/store/akhjsmrrsakcnj8x3xgygvizhccbyn0v-go-1.19.3/share/go/src/fmt/print.go:65 [....] It's a stack overflow due to recursion ``` <details><summary>Contributors' checklist...</summary> - [ ] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [ ] Updated the official documentation or not needed - [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [ ] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details>

albttx requested review from jaekwon and moul as code owners September 28, 2023 09:55

github-actions bot assigned albttx Sep 28, 2023

github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Sep 28, 2023

albttx force-pushed the fix/simple-client-get-account branch from d6f0545 to 97fa258 Compare September 28, 2023 09:56

albttx force-pushed the fix/simple-client-get-account branch from 97fa258 to dbd94d4 Compare September 28, 2023 13:03

albttx mentioned this pull request Sep 28, 2023

chore(amino): improve error readability #1179

Merged

7 tasks

thehowl approved these changes Oct 4, 2023

View reviewed changes

zivkovicmilos mentioned this pull request Oct 5, 2023

amino.Unmarshal doesn't work without unused import #1197

Open

zivkovicmilos approved these changes Oct 5, 2023

View reviewed changes

moul approved these changes Oct 5, 2023

View reviewed changes

moul requested a review from piux2 October 5, 2023 16:39

moul mentioned this pull request Oct 5, 2023

Minutes: GitHub Review Meetings [every Thursday] gnolang/meetings#18

Open

3 tasks

moul linked an issue Oct 5, 2023 that may be closed by this pull request

amino.Unmarshal doesn't work without unused import #1197

Open

piux2 requested changes Oct 6, 2023

View reviewed changes

chore: remove print of typeinfo

cb0ae3e

albttx force-pushed the fix/simple-client-get-account branch from 6dcc0ca to cb0ae3e Compare October 6, 2023 11:07

chore: add package amino test

8aff57b

chore: update crypto imported struct souce

bffccbf

albttx added 2 commits December 7, 2023 12:25

chore: move import into account.go

ac8594e

Merge branch 'master' of github.com:gnolang/gno into fix/simple-clien…

3bef735

…t-get-account

piux2 approved these changes Dec 14, 2023

View reviewed changes

piux2 merged commit eef0c98 into gnolang:master Dec 14, 2023
178 of 179 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: import secp256k1 for amino parsing #1178

chore: import secp256k1 for amino parsing #1178

albttx commented Sep 28, 2023

codecov bot commented Sep 28, 2023 •

edited

Loading

thehowl left a comment

moul left a comment

piux2 left a comment

thehowl commented Oct 10, 2023

piux2 commented Oct 23, 2023 •

edited

Loading

thehowl commented Oct 23, 2023

albttx commented Oct 24, 2023

piux2 commented Oct 25, 2023

thehowl commented Dec 7, 2023

albttx commented Dec 7, 2023

albttx commented Dec 7, 2023

chore: import secp256k1 for amino parsing #1178

chore: import secp256k1 for amino parsing #1178

Conversation

albttx commented Sep 28, 2023

Amino failed to parse crypto.PubKey

Description

codecov bot commented Sep 28, 2023 • edited Loading

Codecov Report

thehowl left a comment

Choose a reason for hiding this comment

moul left a comment

Choose a reason for hiding this comment

piux2 left a comment

Choose a reason for hiding this comment

thehowl commented Oct 10, 2023

piux2 commented Oct 23, 2023 • edited Loading

thehowl commented Oct 23, 2023

albttx commented Oct 24, 2023

piux2 commented Oct 25, 2023

thehowl commented Dec 7, 2023

albttx commented Dec 7, 2023

albttx commented Dec 7, 2023

codecov bot commented Sep 28, 2023 •

edited

Loading

piux2 commented Oct 23, 2023 •

edited

Loading